Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run doctests in pytest. Add linters to pre-commit. Add a pre-commit workflow. #96

Merged

Conversation

aliddell
Copy link
Member

@aliddell aliddell commented Oct 6, 2023

  • Adds doctests for pyi and md files.
  • Pulls pytest arguments used in build / test workflows into pyproject.toml.
  • Adds ruff and rustfmt checks to pre-commit.
  • Adds a pre-commit check workflow that runs on PRs into main.

@aliddell aliddell force-pushed the 92-run-tests-on-documented-examples branch from a48ca40 to 03245c8 Compare October 6, 2023 21:44
README.md Outdated Show resolved Hide resolved
Copy link
Member

@nclack nclack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

I know you've got a question for @andy-sweet and I wonder about other things to get into the pre commit (e.g. ruff and cargo fmt). So feel free to hold off on merging till you're comfortable. But I think this can go in as is.

Copy link
Contributor

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running doctests as part of pre-commit feels a bit heavy to me (e.g. compared to linting), especially when those tests depend on locally connected hardware.

I think that pytest should just pick up on any doctests in Python modules by default and we can modify the glob pattern to also look at the Markdown files (and Rust files if needed). In that case, we don't need to add any new workflows either (though adding a remote pre-commit might be a good idea).

tests/test_docs.py Outdated Show resolved Hide resolved
@aliddell
Copy link
Member Author

I added a check on the .pyi files to test_docs.py and removed the pre-commit check, opting to just call pytest -k test_docs from the usual tests workflow.

Copy link
Contributor

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, but I don't think we need to explicitly collect doctests with test_docs.py, in which case we can and should make a few simplifications.

tests/test_docs.py Outdated Show resolved Hide resolved
.github/workflows/test_pr.yml Outdated Show resolved Hide resolved
.github/workflows/test_pr.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is mostly about adding code format checkers to pre-commit (and the resulting changes that causes) than it is about running the tests on documented examples.

I'm fine with that (i.e. no need to split the PR), but at least update the title and description to match that.

Comment on lines +67 to +74
exclude = [
"__init__.py"
]

ignore = [
"F403",
"F405",
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK to start with these excludes and ignores, though I'd prefer to have none of them in the long term.

@aliddell aliddell changed the title Run tests on documented examples Run doctests in pytest. Add linters to pre-commit. Add a pre-commit workflow. Dec 13, 2023
@aliddell aliddell merged commit c04f714 into acquire-project:main Dec 13, 2023
21 checks passed
@aliddell aliddell deleted the 92-run-tests-on-documented-examples branch December 13, 2023 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants